-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DEPR: Rolling.win_type returning freq & is_datetimelike #38963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this!
Question: is there a reason that it can't keep returning "freq" for the win_type
? (You don't seem to have to change anything else by making self.win_type
return "freq" again.)
Or is it just that we don't need it internally?
pandas/tests/window/test_win_type.py
Outdated
def test_win_type_freq_return_deprecation(): | ||
freq_roll = Series(range(2), index=date_range("2020", periods=2)).rolling("2s") | ||
with tm.assert_produces_warning(FutureWarning): | ||
freq_roll.win_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
freq_roll.win_type | |
assert freq_roll.win_type == "freq" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done
Yes Also since we document in https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.rolling.html
I think its better for consistency that |
OK, that makes sense. Another change that I noticed is the value stored in
vs master:
So But, that also makes it hard to follow the suggestion of "Check the type of self.window instead." in the deprecation warning (since this changed as well, or at least that means you need an actual pandas version check to do something different based on the pandas version, so not insurmountable in the end ;)) |
The doc build is failing, we seem to be checking the
|
Yeah the warning is occurring when a Should I just be suppressing that warning in |
Yeah, either catching/suppressing the warning, or either explicitly checking for the "win_type" attribute and in that case check the window and not include win_type if window is a time-based string? (if that's possible) Either is fine for me. BTW, tested this branch with dask, and the tests are passing again (with warnings). And should be quite straightforward to update it to work on pandas master without warning as well. |
pandas/core/window/rolling.py
Outdated
warnings.simplefilter("ignore", FutureWarning) | ||
super()._shallow_copy(obj, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warnings.simplefilter("ignore", FutureWarning) | |
super()._shallow_copy(obj, **kwargs) | |
with warnings.catch_warnings(): | |
warnings.filterwarnings("ignore", "win_type", FutureWarning) | |
super()._shallow_copy(obj, **kwargs) |
this should ensure the warning filter is only changed inside that context
this should ensure
thanks @mroeschke |
Thanks @mroeschke ! |
@@ -161,6 +161,8 @@ Deprecations | |||
- Deprecated :meth:`MultiIndex.is_lexsorted` and :meth:`MultiIndex.lexsort_depth` as a public methods, users should use :meth:`MultiIndex.is_monotonic_increasing` instead (:issue:`32259`) | |||
- Deprecated keyword ``try_cast`` in :meth:`Series.where`, :meth:`Series.mask`, :meth:`DataFrame.where`, :meth:`DataFrame.mask`; cast results manually if desired (:issue:`38836`) | |||
- Deprecated comparison of :class:`Timestamp` object with ``datetime.date`` objects. Instead of e.g. ``ts <= mydate`` use ``ts <= pd.Timestamp(mydate)`` or ``ts.date() <= mydate`` (:issue:`36131`) | |||
- Deprecated :attr:`Rolling.win_type` returning ``"freq"`` (:issue:`38963`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small future note: since pd.Rolling
does not exist, such a :attr:`Rolling.win_type`
does not work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay thanks for the note.
xref: #38641 (comment)
xref: https://github.com/pandas-dev/pandas/pull/38664/files#r551419130